Added ABC Analysis Setup to Power BI Reports BC app#5869
Added ABC Analysis Setup to Power BI Reports BC app#5869tuan-nguyen-fenwick wants to merge 20 commits intomicrosoft:mainfrom
Conversation
|
@mynjj Can you review this PR for Power BI Reports BC app please? |
|
Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link. |
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
There was a problem hiding this comment.
From what I see that the initialization does, we could do this on the fly instead of at upgrade. Let's do that, let's say when opening the main setup page, and if the setup is needed and it's not there, return default values.
There was a problem hiding this comment.
+1 for this approach.
There was a problem hiding this comment.
@tuan-nguyen-fenwick , what do you think about this suggestion? to initialize on the fly instead of needing to add upgrade code
src/Apps/W1/PowerBIReports/App/Inventory/Embedded/Inventory/ABCAnalysis.Page.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
| var | ||
| SetupHelper: Codeunit "Power BI Report Setup"; | ||
| ReportId: Guid; | ||
| ReportPageLbl: Label 'a476d6afc8d5d544193b', Locked = true; |
There was a problem hiding this comment.
this report id doesn't seem to be in yet, should we include it in this PR? I'm also fine if you want to keep it in a separate PR
src/Apps/W1/PowerBIReports/App/Inventory/Tables/ABCAnalysisSetup.Table.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Pages/ABCAnalysisSetup.Page.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/PowerBIReports/App/Inventory/Tables/ABCAnalysisSetup.Table.al
Outdated
Show resolved
Hide resolved
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
There was a problem hiding this comment.
@tuan-nguyen-fenwick , what do you think about this suggestion? to initialize on the fly instead of needing to add upgrade code
| tabledata "PowerBI Reports Setup" = RIMD, | ||
| tabledata "Working Day" = RIMD, | ||
| tabledata "Account Category" = RM; | ||
| tabledata "Account Category" = RMID, |
There was a problem hiding this comment.
why did we add the "ID" to account category for this functionality?
| tabledata "Working Day" = RIMD, | ||
| tabledata "Account Category" = RM; | ||
| tabledata "Account Category" = RMID, | ||
| tabledata "PowerBI ABC Analysis Setup" = RMID; |
There was a problem hiding this comment.
very minor 😅 : it's usually RIMD (in that order)
src/Apps/W1/PowerBIReports/App/Inventory/Embedded/Inventory/ABCAnalysis.Page.al
Outdated
Show resolved
Hide resolved
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Inventory.PowerBIReports; | ||
|
|
||
| page 36982 "PowerBI ABC Analysis Setup" |
There was a problem hiding this comment.
same here, you were missing to rename the file
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Inventory.PowerBIReports; | ||
|
|
||
| table 36956 "PowerBI ABC Analysis Setup" |
|
|
||
| local procedure InitializeABCAnalysisSetupUpgradeTag(): Code[250] | ||
| begin | ||
| exit('MS-TEMP-POWERBI-ABC-ANALYSIS-SETUP-20251216'); //TODO: To be confirmed with Microsoft team. |
There was a problem hiding this comment.
Ok, so you need workitem No. It is 606229. The code could be like this:
exit('MS-606229-PowerBIABCAnalysisSetup-20251216')
| UpgradeTag.SetUpgradeTag(CloseIncomeSourceCodeUpgradeTag()); | ||
| end; | ||
|
|
||
| local procedure InitializeABCAnalysisSetupUpgrade() |
There was a problem hiding this comment.
+1 for this approach.
PredragMaricic
left a comment
There was a problem hiding this comment.
Please fix comments above
@PredragMaricic Thank you, I've updated the upgrade tag. All other comments can be resolved. |
|
@PredragMaricic |
Prerequisite
Requires PR completion: https://github.com/microsoft/BusinessCentralApps/pull/1757
Change summary
New ABC Analysis page for Inventory Power BI Report:
Fixes AB#606229